-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SDK-3469 Remove deprecated purchase methods #1711
SDK-3469 Remove deprecated purchase methods #1711
Conversation
Co-authored-by: Cesar de la Vega <[email protected]>
…InfoAPI. I should not blindly commit suggestions from GitHub :D
Generated by 🚫 Danger |
...es/purchase-tester/src/main/java/com/revenuecat/purchasetester/DeprecatedOfferingFragment.kt
Outdated
Show resolved
Hide resolved
@@ -114,7 +114,7 @@ internal class PurchasesTest : BasePurchasesTest() { | |||
// region purchasing | |||
|
|||
@Test | |||
fun `deprecated upgrade defaults ProrationMode to null if not passed`() { | |||
fun `upgrade defaults ReplacementMode to WITHOUT_PRORATION if not passed`() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this test to assert the current behavior, assuming that that behavior is correct.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## sdk-3465-support-bc7 #1711 +/- ##
=======================================================
Coverage ? 82.86%
=======================================================
Files ? 220
Lines ? 7540
Branches ? 1066
=======================================================
Hits ? 6248
Misses ? 874
Partials ? 418 ☔ View full report in Codecov by Sentry. |
"Use purchase() and PurchaseParams.Builder instead", | ||
ReplaceWith("purchase()"), | ||
) | ||
fun purchaseProduct( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO I would leave these functions that don't have a UpgradeInfo
. I think some people might still be using those, and we should try to minimize the changes if possible. Lmk if you think otherwise though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were deprecated in the current major #825, that's where my suggestion for removal came from. If we don't remove them now, they will live for another major. Current major was released in September, I think 9 months is enough time to migrate to the purchase
function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but I think it shouldn't be a big problem to keep those around right? And I think as long as it's not a problem, we should keep the deprecated functions. If at some point it becomes a problem, we can reconsider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I see your point. For sure it's not a big problem to keep them around, my suggestion to remove them just comes from the fact that we normally remove deprecated functions in the next major, at least that's what we have been doing.
If we don't do some cleanup from time to time, it can become a huge mess to keep deprecated functions around forever, along with its kotlin conversions and their tests. They are also functions that won't be used in the docs snippets, possibly increasing our support burden, from people still using them.
IMO, I think if that's something we are going to start doing (keep deprecated functions forever as long as we can convert them to something non-deprecated), we should discuss it as a team and it should apply to all SDKs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I added it to our sync. Let's discuss it then!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spoke to @tonidero yesterday and have concluded that it is probably better to keep them, so we focus on the developers and not on our own development experience. We can always remove them in the future if they become a pain to maintain. I think it's still worth to discuss in the meeting today so we all align for the future though.
Sorry for the back and forth, and for making you work on something that is getting scratched 😢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, it's good exercise! 😄 I'll await today's meeting just in case, and will make the changes afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now done!
…urchase-methods # Conflicts: # api-tester/src/main/java/com/revenuecat/apitester/java/UpgradeInfoAPI.java # api-tester/src/main/java/com/revenuecat/apitester/kotlin/UpgradeInfoAPI.kt # examples/purchase-tester/src/main/java/com/revenuecat/purchasetester/DeprecatedOfferingFragment.kt # purchases/src/defaults/kotlin/com/revenuecat/purchases/Purchases.kt # purchases/src/main/kotlin/com/revenuecat/purchases/UpgradeInfo.kt # purchases/src/main/kotlin/com/revenuecat/purchases/common/Backend.kt # purchases/src/test/java/com/revenuecat/purchases/common/backend/BackendTest.kt
### Description PR to support pending prepaid purchase as a new feature added in BC7. This will be enabled by default but can be disabled through an option in `PurchasesConfiguration`: `pendingTransactionsForPrepaidPlansEnabled `.
This reverts commit 3003da6.
This reverts commit bb44815.
This reverts commit 46e6fb0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great!
@@ -38,12 +39,13 @@ class DeprecatedPackageCardAdapter( | |||
binding.currentPackage = currentPackage | |||
binding.isSubscription = product.type == ProductType.SUBS | |||
binding.isActive = activeSubscriptions.contains(product.id) | |||
// Upgrades are no longer possible with deprecated methods. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I think it might make sense to just remove these deprecated classes now... I believe it was mostly used to test during the migration to the new ReplacementMode, but I think it wouldn't be very useful now... But this can also happen separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright! My vote would be to do it separately indeed, to avoid this PR from creeping up in scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also quicker v8 beta 🚀 😄
This PR leaves just
purchase(PurchaseParams)
as the only way to purchase.Breaking changes
purchasePackage()
purchaseProduct()
purchasePackageWith()
purchaseProductWith()
UpgradeInfo